src/buffer.cpp: Avoid CodeQL warning about multiplication overflow #2292
Conversation
|
At least the warning in the files tab disappeared. Hopefully changing the type doesn’t introduce any bugs. But I doubt that. CodeQL still seems to find issues, but I can’t find them. Probably we’ll see after a merge |
There was a problem hiding this comment.
The fix looks correct to me (after having read some background).
As I understand, this PR changes the type of two loop variables in a way so that a later multiplication involving this loop variable is forced to occur in the larger type (long) instead of the type of all the operands (int).
I guess one other, potentially more readable solution would be casting one of the operands to long in place, but I assume that this is -- at least theoretically -- less efficient instead of doing it once per loop as you do.
I guess it could use a comment to avoid later "improvements" where someone wonders why it is a long and reverts back to int. See suggestions.
CHANGELOG: Internal: A hypothetical CodeQL-detected multiplication overflow in sound buffer handling has been fixed.
hoffie
changed the title
Co-authored-by: Christian Hoffmann <christian@hoffie.info>
|
In the longer term, I would prefer to eliminate multiplications within array indices. I prefer to traverse with suitably-incremented simple indices or pointers. But the immediate need was to eliminate the red warnings in Github. |
This is more efficient, and should avoid the CodeQL warnings about multiplication overflow within array indices or vector offsets in buffer.cpp and server.cpp Replaces the changes that were done in jamulussoftware#2292
Short description of changes
Satisfy CodeQL multiplication overflow warnings by changing a loop variable from
inttolongContext: Fixes an issue?
Fixes some high-severity code alerts raised by Github, even though they wouldn't be encountered in practice.
Does this change need documentation? What needs to be documented and how?
No documentation needed
Status of this Pull Request
Trivial change, ready to merge
What is missing until this pull request can be merged?
Validate that the warnings on
src/buffer.cppare no longer raised by GithubChecklist